[Mypy] Add more mypy #1564

Michaelvll · 2022-12-30T07:40:32Z

This PR adds mypy checks for almost all the modules excluding the following files:

--exclude sky/benchmark
--exclude sky/callbacks
--exclude sky/skylet/providers # blocked by the ray upstream
--exclude sky/resources.py # blocked by #1429 
--exclude sky/global_user_state.py
--exclude sky/optimizer.py
--exclude sky/task.py

Except for the modules with comments above, we will add mypy checks for the rest of the files in a future PR.

Tested (run the relevant ones):

Any manual or new tests for this PR (please specify below)
- mypy checks pass
All smoke tests: bash tests/run_smoke_tests.sh
Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

…nto more-mypy

romilbhardwaj

Thanks @Michaelvll! I've reviewed 16/36 files (up to execution.py). I can do more reviews late next week, but I would like someone else to look at this if they can get to it before 19th Jan.

romilbhardwaj · 2023-01-13T20:06:12Z

format.sh

@@ -98,8 +98,6 @@ mypy $(cat tests/mypy_files.txt)
 # Run Pylint
 echo 'Sky Pylint:'
 pylint --load-plugins pylint_quotes sky


on running format.sh, I got an error:

(base) romilb@romilbx1yoga:/mnt/d/Romil/Berkeley/Research/sky-experiments$ ./format.sh SkyPilot mypy: sky/authentication.py:187: error: No overload variant of "get" of "dict" matches argument types "str", "str" [call-overload] sky/authentication.py:187: note: Possible overload variants: sky/authentication.py:187: note: def get(self, <nothing>, /) -> None sky/authentication.py:187: note: def [_T] get(self, <nothing>, _T, /) -> _T sky/authentication.py:249: error: No overload variant of "get" of "dict" matches argument types "str", "str" [call-overload] sky/authentication.py:249: note: Possible overload variants: sky/authentication.py:249: note: def get(self, <nothing>, /) -> None sky/authentication.py:249: note: def [_T] get(self, <nothing>, _T, /) -> _T Found 2 errors in 1 file (checked 83 source files)

Here's my pip freeze if it helps

Hmm, that is weird. Could you share the python version you are using?

sky/backends/backend_utils.py

romilbhardwaj · 2023-01-14T01:47:19Z

sky/backends/cloud_vm_ray_backend.py

@@ -2102,7 +2101,7 @@ def _provision(self,
                        raise exceptions.ResourcesUnavailableError(
                            error_message) from None
            if dryrun:
-                return
+                return None


Should we change other instances of return to return None to be consistent? (as suggested in pep8 - "Be consistent in return statements"). E.g. at L2277

I added the return None for the LocalDockerBackend._provision as well, but for the return statement at L2277, the method has a return type annotation as -> None, so it seems to be fine (or more intuitive) to not having the return value in the return statement.

Seems the standard for being consistent for the return statement has a scope of each function, IIUC. It might be fine to only have the return statement consistent in a single function.

Either all return statements in a function should return an expression, or none of them should.

sky/cli.py

sky/core.py

…nto more-mypy

Michaelvll · 2023-01-15T09:16:24Z

With the latest changes, I avoid the nested class of the ResourceHandle as we are using the type of the handle outside the outer class. There is no reason to keep it nested in the Backend.

…nto more-mypy

Michaelvll · 2023-02-14T18:12:34Z

A kind reminder for this PR @romilbhardwaj. : )

romilbhardwaj

Thanks @Michaelvll! Sorry about the delay in getting to this. Left some comments, particularly need some more on clarity on why pyi stub files are needed. Rest looks great!

sky/setup_files/setup.py

sky/skylet/log_lib.py

romilbhardwaj · 2023-02-21T02:00:57Z

sky/skylet/log_lib.pyi

@@ -0,0 +1,108 @@
+from sky import sky_logging as sky_logging


I'm a little new to mypy, but is this stub file necessary if we fully type log_lib.py?

This stub file is mainly for the type hint of run_with_log where we want to determine the return type by the value of the argument require_outputs. To remove this stub file, we can move the two typing.overload back into the original log_lib.py, but that may make that file much longer. Wdyt?

Ahh... I think it would be nicer to move the overload definitions to log_lib.py (fewer files to deal with, easier to maintain).

On a related note - should we reconsider why we have different return types for run_with_log? It would be nice if we can get away with Tuple[int, str, str] and return proc.returncode, '', '' when require_outputs is false? Feel free to defer for later too

(Casual skim, didn't read the discussion in full) .pyi files being standalone seems common practice:

https://github.com/tensorflow/tensorflow/blob/master/tensorflow/compiler/xla/python/xla_client.pyi

https://github.com/tensorflow/tensorflow/blob/master/tensorflow/compiler/xla/python/xla_extension/ops.pyi

https://github.com/ray-project/ray/blob/master/python/ray/_raylet.pyi

Oops, sorry. Just realized, another reason we have to use the stub file is that the pylint has a bug with the type overload in the same file (reference). It will not correctly recognize the overloaded types and complain about sky/backends/cloud_vm_ray_backend.py:1362:8: E0633: Attempting to unpack a non-sequence (unpacking-non-sequence).
I tried to upgrade the pylint to the latest version but the problem still exists. I had to keep the stub file for now.

I am planning to refactor the require_outputs and remove the stub file in another PR as otherwise, the changes will be mixed with the mypy changes in this PR, making it hard to distinguish the changes. : )

sky/spot/spot_utils.py

sky/utils/command_runner.pyi

…nto more-mypy

romilbhardwaj

Thanks for adding this @Michaelvll! This will help us catch bugs a lot faster : ) Should be good to merge if all tests pass.

romilbhardwaj · 2023-02-24T16:13:12Z

sky/backends/backend_utils.py

-SKY_RESERVED_CLUSTER_NAMES = {
-    spot_lib.SPOT_CONTROLLER_NAME: 'Managed spot controller'
+SKY_RESERVED_CLUSTER_NAMES: Dict[str, str] = {
+    spot_lib.SPOT_CONTROLLER_NAME: 'Managed spot controller'  # type: ignore


This # type: ignore seems not required - mypy passes without this too

Oops, good catch! Removed it. Thanks!

…nto more-mypy

Michaelvll · 2023-02-24T17:13:07Z

Thanks for the review @romilbhardwaj! Tested:

pytest tests/test_smoke.py
sky commands in a new container.

* backend_utils mypy * More mypy * format * order * change to exclude * remove unused registry.py * add back task.py * dependency * quote * fix circular import * fix * remove unused * fix issue with master * refactor ResourceHandle * fix * dryrun for local docker backend * lint * fix merge issue * Address comments * lint * format * remote type ignore * add typing extensions to the dependency

Michaelvll added 11 commits December 28, 2022 23:34

backend_utils mypy

5568587

More mypy

e112cd3

format

a98ed69

order

11f2c75

change to exclude

8289dbc

remove unused registry.py

75c8ddd

add back task.py

98cf7a4

dependency

67b3fda

quote

31a99c3

fix circular import

a2e0075

fix

bf75ee5

Michaelvll marked this pull request as ready for review December 30, 2022 19:53

remove unused

fdb65fd

Michaelvll requested a review from romilbhardwaj December 31, 2022 23:36

Michaelvll mentioned this pull request Jan 3, 2023

[API] Add support for specifying the clusters to query #1568

Merged

1 task

Merge branch 'master' of github.com:concretevitamin/sky-experiments i…

96da35e

…nto more-mypy

Michaelvll requested a review from infwinston January 3, 2023 20:52

Michaelvll added 3 commits January 10, 2023 18:36

Merge branch 'master' of github.com:concretevitamin/sky-experiments i…

9483dab

…nto more-mypy

fix issue with master

0715076

Merge branch 'master' of github.com:concretevitamin/sky-experiments i…

9f3e2e7

…nto more-mypy

Michaelvll requested a review from concretevitamin January 12, 2023 21:25

concretevitamin removed request for concretevitamin and infwinston January 13, 2023 01:16

romilbhardwaj reviewed Jan 14, 2023

View reviewed changes

romilbhardwaj requested a review from infwinston January 14, 2023 07:47

Michaelvll added 5 commits January 14, 2023 18:45

Merge branch 'master' of github.com:concretevitamin/sky-experiments i…

2ee2081

…nto more-mypy

refactor ResourceHandle

5672f7c

fix

8f8f07b

dryrun for local docker backend

9685d09

lint

78af26a

Michaelvll added 2 commits January 22, 2023 00:02

Merge branch 'master' of github.com:concretevitamin/sky-experiments i…

ddac3dc

…nto more-mypy

fix merge issue

e751f49

Michaelvll requested a review from romilbhardwaj January 22, 2023 08:10

Michaelvll mentioned this pull request Jan 24, 2023

[Azure] Fix azure launch #1619

Merged

1 task

romilbhardwaj mentioned this pull request Jan 27, 2023

[core] Refactoring the return value of teardown #1595

Merged

1 task

Merge branch 'master' of github.com:concretevitamin/sky-experiments i…

093889c

…nto more-mypy

Michaelvll mentioned this pull request Feb 14, 2023

[Azure/Core] Fix blocklist for azure #1695

Merged

4 tasks

romilbhardwaj reviewed Feb 21, 2023

View reviewed changes

Merge branch 'master' of github.com:concretevitamin/sky-experiments i…

7242b64

…nto more-mypy

Michaelvll force-pushed the more-mypy branch from bf79d5c to 7242b64 Compare February 21, 2023 04:14

Michaelvll added 4 commits February 20, 2023 20:15

Address comments

17bc41f

lint

23b84b2

Merge branch 'master' of github.com:concretevitamin/sky-experiments i…

59a14c1

…nto more-mypy

format

374ba72

romilbhardwaj approved these changes Feb 24, 2023

View reviewed changes

Michaelvll added 2 commits February 24, 2023 09:11

remote type ignore

f682eaf

Merge branch 'master' of github.com:concretevitamin/sky-experiments i…

e085f4a

…nto more-mypy

add typing extensions to the dependency

278960d

Michaelvll merged commit 22397b1 into master Feb 24, 2023

Michaelvll deleted the more-mypy branch February 24, 2023 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Mypy] Add more mypy #1564

[Mypy] Add more mypy #1564

Michaelvll commented Dec 30, 2022 •

edited

Loading

romilbhardwaj left a comment

romilbhardwaj Jan 13, 2023

Michaelvll Jan 15, 2023

romilbhardwaj Jan 14, 2023

Michaelvll Jan 15, 2023

Michaelvll commented Jan 15, 2023

Michaelvll commented Feb 14, 2023

romilbhardwaj left a comment

romilbhardwaj Feb 21, 2023

Michaelvll Feb 21, 2023

romilbhardwaj Feb 21, 2023 •

edited

Loading

concretevitamin Feb 22, 2023

Michaelvll Feb 22, 2023 •

edited

Loading

romilbhardwaj left a comment

romilbhardwaj Feb 24, 2023

Michaelvll Feb 24, 2023

Michaelvll commented Feb 24, 2023 •

edited

Loading

[Mypy] Add more mypy #1564

[Mypy] Add more mypy #1564

Conversation

Michaelvll commented Dec 30, 2022 • edited Loading

romilbhardwaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michaelvll commented Jan 15, 2023

Michaelvll commented Feb 14, 2023

romilbhardwaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romilbhardwaj Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michaelvll Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

romilbhardwaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michaelvll commented Feb 24, 2023 • edited Loading

Michaelvll commented Dec 30, 2022 •

edited

Loading

romilbhardwaj Feb 21, 2023 •

edited

Loading

Michaelvll Feb 22, 2023 •

edited

Loading

Michaelvll commented Feb 24, 2023 •

edited

Loading